Settings section improvement #918
Conversation
…ppear when key already exists
| @@ -285,6 +274,147 @@ export function ChatSettings(props: ChatSettingsProps): JSX.Element { | |||
| ); | |||
| } | |||
|
|
|||
| let language_model_section; | |||
There was a problem hiding this comment.
I think snakeCase would be more consistent with the codebase here (and for the other one). Not sure what others think about using a variable vs keeping it inline with ternary vs moving it to a new functional component.
Co-authored-by: Michał Krassowski <[email protected]>
Co-authored-by: Michał Krassowski <[email protected]>
Co-authored-by: Michał Krassowski <[email protected]>
There was a problem hiding this comment.
Tested it locally, and I am not able to select model right now because the current version checks the current provider rather than all of them (and there are times when there is no provider configured, like when launching the extension for the first time).
For other reviewers, you can test the "all disabled state" this with
jupyter lab --AiExtension.allowed_providers=X --AiExtension.allowed_providers=X
which disallows all providers (needs to be repeated twice due to #913).
| @@ -285,6 +274,150 @@ export function ChatSettings(props: ChatSettingsProps): JSX.Element { | |||
| ); | |||
| } | |||
|
|
|||
| let languageModelSection; | |||
| if (lmProvider?.chat_models && lmProvider.chat_models.length > 0) { | |||
There was a problem hiding this comment.
This needs to be iterated across all server.lmProviders, not only the current one, in a similar why to how the menu options are populated, see:
{server.lmProviders.providers.map(lmp =>
lmp.models
.filter(lm => lmp.chat_models.includes(lm))
.map(lm => (
<MenuItem value={`${lmp.id}:${lm}`}>
{lmp.name} :: {lm}
</MenuItem>
))
)}There was a problem hiding this comment.
This should be fixed now.
| lmProvider?.completion_models && | ||
| lmProvider?.completion_models.length > 0 |
|
@andrewfulton9 TY for working on this. Please also update the related documentation for this enhancement. I have also tested the changes and they seem to be working correctly, with one clarification needed, maybe @krassowski would know best. When only the "completions History provider" is enabled, the box on the left to enter the Inline completion model is grayed out, and is only active when the "completions Jupyter AI provider" is enabled and that box is checked as can be seen next: |
Yes, this is exepcted - the history provider is from core jupyterlab. The sidebar should only react to changes in "Jupyter AI provider" status. |
There was a problem hiding this comment.
@andrewfulton9 Thank you for this PR! Mike and I have left some feedback below to address.
In addition, rather than defining languageModelSection and inlineCompletionSection as variables storing JSX elements, can you handle the conditional logic directly within the return statement using ternary statements? This is like what is being done in lines 444-466 for the embedding models section. Doing so would improve readability IMO, since it does not require a reader to jump above to the definition of languageModelSection to determine how the output is being rendered.
for more information, see https://pre-commit.ci
…n9/jupyter-ai into settings_section_improvement
|
@srdas, thanks for reviewing! Do you have a suggestion on what in the docs should be updated? I don't see anything in the docs related to the settings panel. Maybe something in the "blocklisting providers" section? Though this PR doesn't actually do any configuration, it just makes the settings panel more clear when a there isn't a option available to select. |
Thanks for asking for clarification @andrewfulton9. As of now, inline completion is not discussed in the documentation, see this link: https://jupyter-ai.readthedocs.io/en/latest/users/index.html#the-chat-interface. In this section in the User documentation, after discussing the choice of Language model and Embedding model, the screenshots shown should be replaced by (i) showing inline completion as an option, (ii) showing the settings page for Inline Completion, (iii) explain the latest version of the inline completion setup incorporating the improvement in this PR. Wait to do this (and I'd be happy to help) after finalizing the changes in this PR based on comments from @dlqqq and @krassowski at which point the latest version of the UI can be used to update the documentation. |
* show no api keys needed for selected model * checks for existing key in rendering so No API Keys message doesn't appear when key already exists * Handle not having language and embedding models * lint * inline_completer * Update packages/jupyter-ai/src/components/chat-settings.tsx Co-authored-by: Michał Krassowski <[email protected]> * Update packages/jupyter-ai/src/components/chat-settings.tsx Co-authored-by: Michał Krassowski <[email protected]> * Update packages/jupyter-ai/src/components/chat-settings.tsx Co-authored-by: Michał Krassowski <[email protected]> * review fixes * addressing reviews * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * lint --------- Co-authored-by: Michał Krassowski <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Closes #903
This changes the settings section so that the dropdown is replaced by text indicating that the section assets are not available (API Keys, language models, embedded models). It also replaces the inline completer dropdown with text indicating if the inline completer is not enabled.